Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Aligned text in the TextBoxComponent #1620

Merged
merged 40 commits into from
Jun 3, 2022

Conversation

st-pasha
Copy link
Contributor

@st-pasha st-pasha commented May 13, 2022

Description

  • Added option align in the TextBoxComponent which controls the alignment of text.
  • Added option for the TextBoxComponent to have a fixed size (before the only mode was for the textbox to automatically expand/shrink to fit the text).

Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
  • I have read the Contributor Guide and followed the process outlined for submitting PRs.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples.

Breaking Change

  • [-] Yes, this is a breaking change.
  • No, this is not a breaking change.

Related Issues

Closes #1088

Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

doc/flame/rendering/text.md Outdated Show resolved Hide resolved
@st-pasha
Copy link
Contributor Author

@spydon Please take a look at the failing test, I suspect it might be due to differences between Flutter versions on my machine and in the CI pipeline.

@spydon
Copy link
Member

spydon commented May 16, 2022

@spydon Please take a look at the failing test, I suspect it might be due to differences between Flutter versions on my machine and in the CI pipeline.

I would guess that it is due to the classic font rendering problem when using goldens:
https://pub.dev/packages/alchemist#about-platform-tests-vs-ci-tests

Copy link
Member

@erickzanardo erickzanardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Golden tests involving texts are usually a pitfall because text rendering is influenced by the OS where the test is run, so if we generate a snapshot on linux and then try to run the test on MacOs or windows, there are high possibility that the test will fail.

So IMO we shouldn't use golden tests for these situations.

@spydon
Copy link
Member

spydon commented May 16, 2022

Golden tests involving texts are usually a pitfall because text rendering is influenced by the OS where the test is run, so if we generate a snapshot on linux and then try to run the test on MacOs or windows, there are high possibility that the test will fail.

So IMO we shouldn't use golden tests for these situations.

We can use them, but render boxes instead where the text would have been.
Alchemist for example has a mode for this.
(There are also a few fonts that are specialized for golden testing, but that's more work.)

@erickzanardo
Copy link
Member

Alchemist for example has a mode for this.
Interesting, would be cool to test this.

@st-pasha
Copy link
Contributor Author

We can use them, but render boxes instead where the text would have been.

Flutter is already doing this for us: packages/flame/test/_goldens/text_box_component_test_1.png.

So the problem here must be something else. There are diff files generated by the test, but I can't see a way to access them from the CI. Could there be an option somewhere to include those files into the archive with the logs?

Another approach could be to run the test on another local machine with a different version of Flutter and see if the test succeeds or not.

st-pasha and others added 11 commits May 29, 2022 15:16
…ne#1657)

As described flame-engine#1546, It would be more convenient and can reduce the game widget updates if adding or removing overlays at once.
…ngine#1659)

Make sure that subscription to GameStateChange events is properly updated when the game: property of a GameWidget changes.
changeParent should be allowed even when the current parent is null.
Fix the last remaining broken example links.
This was a planned move once we'd switch to Dart 2.15; and we're already on 2.16 since flame-engine#1617.
…-engine#1675)

We were planning to make this change once we upgrade min Dart version to 2.16; which we finally did in flame-engine#1617.
In some places in code we used patterns like fold(0, (a, b) => a + b), which can be replaced with a simple .sum from the Iterable extension.
…lame-engine#1673)

These methods can be simplified using the extension Iterable.firstOrNull getter.
@spydon
Copy link
Member

spydon commented May 30, 2022

I hope that after merging #1634 we can switch to sprite fonts here, and there wouldn't be a problem anymore.

Feels a bit like cheating though, since normal fonts aren't tested then (and this will most likely not be the last time we have this problem in the CI).

@st-pasha
Copy link
Contributor Author

I looked at Alchemist documentation, and they say they use Ahem font when rendering in CI, which replaces all glyphs with blocks. However, Flutter itself uses Ahem font for rendering goldens, yet somehow we see a 1-pixel shift. I'm not sure using Alchemist would solve this...

There's also the golden_toolkit package, and the approach they took is to bundle Roboto font together with the tests, and load it directly from there: https://github.com/eBay/flutter_glove_box/blob/9e881929543e1ed634f854897ded903d4bc2f55a/packages/golden_toolkit/lib/src/font_loader.dart I wonder if we can maybe replicate that somehow?

@spydon
Copy link
Member

spydon commented May 30, 2022

I just noticed that all positions have .5 pixels, maybe this wouldn't cause anti-aliasing if the positions were in .0 instead?
(Just to keep the thread straight, this was already tried)

@spydon
Copy link
Member

spydon commented Jun 3, 2022

@st-pasha Let's put skip: true on the test and open an issue about fixing it, so that this feature can be included in v1.2.0

@st-pasha
Copy link
Contributor Author

st-pasha commented Jun 3, 2022

So, it looks like the issue is caused by different ways that the fonts are smoothed on different operating systems. I made these zoomed-in screenshots, which show that both OSes perform smoothing, but in a different way:
Screen Shot 2022-06-03 at 10 54 24 AM
Screen Shot 2022-06-03 at 10 55 05 AM

I've tried to render the text using a paint with antialiasing turned off - but it didn't make any difference, looks like macOS simply doesn't support switching off antialiasing for fonts (except maybe as a global system option, but that would be too disruptive for day-to-day life)...

In view of this, I believe the best we can do is to not rely on system font rendering altogether and use sprite fonts instead (at least for golden files); or wait until such time as Flutter team figures how to make golden tests match if the difference between images is up to anti-aliasing effects.

@spydon
Copy link
Member

spydon commented Jun 3, 2022

So, it looks like the issue is caused by different ways that the fonts are smoothed on different operating systems. I made these zoomed-in screenshots, which show that both OSes perform smoothing, but in a different way

That is the classic Flutter issue, didn't we post that one here? Otherwise I know @erickzanardo has the link to it somewhere, because he has a workaround for it in that issue.
Alchemist should solve that issue.

@spydon
Copy link
Member

spydon commented Jun 3, 2022

Here is the issue (Erick digged it up):
flutter/flutter#56383
(apparently the suggested workaround doesn't work all the time, so we should avoid that)

@st-pasha
Copy link
Contributor Author

st-pasha commented Jun 3, 2022

because he has a workaround for it in that issue.

The suggested "workaround" - to change the font smoothing at system global level - would only work on a computer that is dedicated solely to running golden tests and to nothing else. Because in a day-to-day life font smoothing is a very much desirable feature, there is a reason companies invest money in improving it.

Of course, ideally, Apple would have an API for rendering a specific string without any antialiasing. But apparently they don't have this, and it's not clear that they ever will (even if they do, we don't want tests that run on some version of OS but not on other, and not everyone can upgrade their OS easily).

In view of this, there may be no other option than manual sprite-based fonts. If Alchemist has a work-around, then this is what they must be doing.

@spydon
Copy link
Member

spydon commented Jun 3, 2022

The suggested "workaround" - to change the font smoothing at system global level - would only work on a computer that is dedicated solely to running golden tests and to nothing else. Because in a day-to-day life font smoothing is a very much desirable feature, there is a reason companies invest money in improving it.

Yeah, sorry I was unclear, I didn't mean that we should use that work around, it was just how I remembered the issue. :)

If Alchemist has a work-around, then this is what they must be doing.

Indeed, let's investigate that post v1.2.0.

@erickzanardo
Copy link
Member

The suggested "workaround" - to change the font smoothing at system global level - would only work on a computer that is dedicated solely to running golden tests and to nothing else. Because in a day-to-day life font smoothing is a very much desirable feature, there is a reason companies invest money in improving it.

Yeah, that workaround was something that used to work, but since some time isn't accurate anymore. There is only one workaround that I think is to be trusted. Running golden tests on the same environment as the CI will run them. On a multi platform world, docker would be an alternative for that, but that would render a very bad developer experience IMO.

In view of this, there may be no other option than manual sprite-based fonts. If Alchemist has a work-around, then this is what they must be doing.

I am a little bit skeptical about this still, giving the nature of the issue. But I would love to see this in action and would love even more to be proven wrong. Lets try to plan for doing this, even if as a POC for now.

@spydon spydon merged commit c64aeda into flame-engine:main Jun 3, 2022
@st-pasha st-pasha deleted the ps/aligned-text branch June 3, 2022 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TextBoxComponent should have the possiblity to align text within itself
4 participants